Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor http response status equatable #335

Merged

Conversation

pedantix
Copy link
Contributor

Refactored extension HTTPResponseStatus: Equatable, no change to the efficacy of the code.

Motivation:

Code readability is essential to a long-lasting maintainable project, the code this PR changes was "readable" however this PR will enhance the readability by removing excess lines of code.

Modifications:

Changed a long case statement, to a much shorter one.

Result:

There will be fewer lines of code, no change to the efficacy of product should occur. I am not familiar enough with the compiler to know if this will have a deleterious effect on performance I don't think it should be based on the way public var code: UInt { get } is implemented.

@swift-nio-bot
Copy link

Can one of the admins verify this patch?

1 similar comment
@swift-nio-bot
Copy link

Can one of the admins verify this patch?

@pedantix
Copy link
Contributor Author

@helje5 pulling this convo in from #327.

so custom(418) is equal to .teapot, but custom(418,”earl gray”) differs to custom(418,”whatever”) 🤔

are you stating that custom anything lhs/rhs should be cased out, I can see the motivation for that. It is still much shorter then long case statement. I will make change.

Copy link
Contributor

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would intuit that:

"404 Not Found" == "404 not found" // true

Why not just compare the code?

@pedantix
Copy link
Contributor Author

@tanner0101 I completely agree from a web perspective the HTTP code is all that matters not the text description as localization could very well cause problems there. I think this would alter the enum's current behavior though. Further, the refactor could probably be reduced to just the enum conforming to the interface.

@normanmaurer
Copy link
Member

I think what we have here is mimic the same as we had before. If we want to change this another PR should be done IMHO.

@normanmaurer
Copy link
Member

@swift-nio-bot test this please

@helje5
Copy link
Contributor

helje5 commented Apr 19, 2018

@pedantix IMO using a closed enum in a open API is deeply flawed in the first place, as demonstrated by the teapot (apart from the SemVer major source incompatibility you introduce by adding a case, the ugliness of custom, and all the bugs revolving around the .custom pattern).
Wrt the specific question: yes, the reason must be ignored. Why? Because it is already is stripped by the parser! I.e. you don't actually get the original reason for "builtin codes" and hence can't know whether it matches some other reason you got.

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I think we should keep the current behaviour of the equatable enum on custom codes.

@BasThomas
Copy link
Contributor

What could be done is to group all the cases that return true to one, and have only one line that returns true, without resorting to a default case.

@pedantix
Copy link
Contributor Author

@Lukasa does the code in my PR not keep the current behavior? As I read it, there is an ever-growing number of cases that pattern matches on if custom compares stored values, if not compare match on equal symbols(which is the source of the growth) if not default to false. I may be missing a swift subtlety but my PR is not aimed at changing functionality just reducing growth. There is definitely a weakness with using a default case. As @BasThomas says all the true pairs could be reduced to a single case, I am not sure if this is any better in terms of readability.

@Lukasa
Copy link
Contributor

Lukasa commented Apr 20, 2018

Sorry, you're right. :)

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weissi Mind taking a look?

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the massive delay! thanks very much for the patch, lgtm!

@weissi
Copy link
Member

weissi commented Apr 24, 2018

@swift-nio-bot test this please

@normanmaurer normanmaurer merged commit 163827b into apple:master Apr 24, 2018
@normanmaurer normanmaurer self-assigned this Apr 24, 2018
@normanmaurer normanmaurer added this to the 1.6.0 milestone Apr 24, 2018
@normanmaurer
Copy link
Member

@pedantix thanks!

@Lukasa Lukasa added the semver/patch No public API change. label Apr 24, 2018
@pedantix
Copy link
Contributor Author

@normanmaurer @weissi @Lukasa thank you! -119 lines of code as a net gain I think is just cool. This project is so great to make use of thanks!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants